storage/eloq/ha_eloq.cc (2)

4876-4883: Potential tight loop; clarify ScanBatchTxRequest::Result() semantics and add progress guard

If Result() returns false and no tuples are returned for a plan, current_index never advances, causing a spin. Please confirm that tx side guarantees progress for the same plan (e.g., save_point advances internally). If not, add a guard to break or back off when no progress is observed.

Example minimal guard:

• Track last_seen_count; if scan_batch.empty() for N consecutive iterations with same current_index, break and log.

Also, consider reading prev_pause_idx_ semantics: if it refers to the “next” plan index, you might want to start from prev_pause_idx_ + 1 to avoid re-scanning.

I recommend:

• Document Result() meaning in a comment.
• Add a no-progress safeguard.

Also applies to: 4903-4924

---

7331-7506: Pushdown type refactor LGTM; small fixes to improve robustness

• Good: Switch to txservice::DataStoreSearchCond and DataStoreDataType aligns with new API.
• Suggest: Expand literal type check to include MYSQL_TYPE_VAR_STRING so varchar constants don’t get skipped.
• Bug: assert(true && "Unsupported cmp function.") never fires. Use assert(false && ...) or remove and handle via default.

Diff sketch:

-      pushdown= (val_field->field_type() == MYSQL_TYPE_STRING ||
-                 val_field->field_type() == MYSQL_TYPE_VARCHAR)
+      pushdown= (val_field->field_type() == MYSQL_TYPE_STRING ||
+                 val_field->field_type() == MYSQL_TYPE_VARCHAR ||
+                 val_field->field_type() == MYSQL_TYPE_VAR_STRING)
                  ? true
                  : false;
...
-    default:
-      assert(true && "Unsupported cmp function.");
-      break;
+    default:
+      assert(false && "Unsupported cmp function.");
+      break;

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

storage/eloq/ha_eloq.cc (1)

4870-4895: Fix OOB: PickPlan must occur after bounds check

You still call save_point->PickPlan(current_index) before verifying current_index < plan_size. If prev_pause_idx_ equals plan_size, this dereferences past end. Move PickPlan inside the while and stop re-picking within the body.

Apply this diff:

-    size_t plan_size= save_point->PlanSize();
-    BucketScanPlan plan= save_point->PickPlan(current_index);
-    while (current_index < plan_size)
+    size_t plan_size= save_point->PlanSize();
+    while (current_index < plan_size)
     {
+      BucketScanPlan plan= save_point->PickPlan(current_index);
       scan_batch.clear();
       ScanBatchTxRequest scan_batch_req(scan_alias, scan_table_name,
                                         &scan_batch, yield_func, resume_func,
-                                        txm, -1, {}, &plan);
+                                        txm, -1, {}, &plan);
       txm->Execute(&scan_batch_req);
       scan_batch_req.Wait();
       if (scan_batch_req.IsError())
       {
         ret= HA_ADMIN_FAILED;
         break;
       }
-
-      if (scan_batch_req.Result())
-      {
-        current_index++;
-        if (current_index < plan_size)
-        {
-          plan= save_point->PickPlan(current_index);
-        }
-      }
+      if (scan_batch_req.Result()) {
+        current_index++;
+      }
     }

storage/eloq/ha_eloq.cc (1)

7303-7478: Pushdown safety and small robustness tweaks in BindPushedCond

• Skip NULL constants to avoid meaningless “col op NULL” pushdowns.
• For VARCHAR/TEXT with '=' you switch to '>=' only; add an upper bound (“< nextPrefix(val)”) to avoid scanning supersets while keeping correctness.
• Minor: reserve result capacity.

Minimal changes:

 std::vector<txservice::DataStoreSearchCond> ha_eloq::BindPushedCond()
 {
+  // Reserve to reduce reallocs
+  std::vector<txservice::DataStoreSearchCond> res;
+  res.reserve(pushed_conds_.size());
@@
     Item_field *col_field= pushed_cond.col_field_;
     Item_literal *val_field= pushed_cond.val_field_;
+    // Do not push down comparisons with NULL
+    if (val_field->null_value) {
+      continue;
+    }
@@
-    switch (func_type)
+    switch (func_type)
     {
     case Item_func::EQ_FUNC:
       op.append("=");
       break;
@@
     }
@@
     switch (field_type.data_type_)
     {
@@
     default: {
       StringBuffer<MAX_FIELD_WIDTH> str;
       field->val_str(&str);
       val_str.append((char *) str.ptr(), str.length());
       // VARCHAR/TEXT type
       if (field_type.data_type_ == EloqDataType::VarString)
       {
         // Remove trailing space.
         size_t endpos= val_str.find_last_not_of(' ');
         if (endpos != std::string::npos && endpos < val_str.length() - 1)
         {
           val_str.replace((endpos + 1), (val_str.length() - endpos - 1), "");
         }
-        // Fetch values from kv without regard for trailing sapces.
-        if (!op.compare("="))
-        {
-          op.assign(">=");
-        }
+        // For '=' push both lower and upper bounds to avoid supersets:
+        // >= trimmed and < next prefix
+        if (op == "=") {
+          // Lower bound (>=)
+          res.push_back({field_name, std::string(">="), val_str, data_type});
+          // Upper bound: next lexicographic prefix
+          std::string hi = val_str;
+          hi.push_back('\xFF');
+          res.push_back({field_name, std::string("<"), hi, data_type});
+          // Restore original field and continue (already pushed both)
+          field->move_field(table->record[0] + field_offset,
+                            maybe_null ? table->record[0] + null_offset : nullptr,
+                            field->null_bit);
+          continue;
+        }
       }
       break;
     }
     }
@@
-    res.push_back({field_name, op, val_str, data_type});
+    res.push_back({field_name, op, val_str, data_type});

Note: nextPrefix via appending 0xFF is acceptable under binary collations used for pushdown here. If non-binary collations are later allowed, use the key-def ceiling helper to compute a safe bound.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

storage/eloq/ha_eloq.cc (1)

4870-4872: Critical: PickPlan before bounds check can read past end (still present)

If current_index == plan_size (e.g., prev_pause_idx_ equals PlanSize), PickPlan(current_index) dereferences out of range. Move PickPlan inside the while after verifying bounds. Also drop the mid-loop reassignment.

Apply:

-    size_t plan_size= save_point->PlanSize();
-    BucketScanPlan plan= save_point->PickPlan(current_index);
-    while (current_index < plan_size)
+    size_t plan_size= save_point->PlanSize();
+    while (current_index < plan_size)
     {
+      BucketScanPlan plan = save_point->PickPlan(current_index);
       scan_batch.clear();
       ...
-      if (scan_batch_req.Result())
-      {
-        current_index++;
-        if (current_index < plan_size)
-        {
-          plan= save_point->PickPlan(current_index);
-        }
-      }
+      if (scan_batch_req.Result()) {
+        current_index++;
+      }
     }

Also applies to: 4889-4893

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.

Learnt from: githubzilla
PR: eloqdata/eloqsql#127
File: concourse/scripts/build_tarball_open.bash:136-136
Timestamp: 2025-09-25T11:33:33.221Z
Learning: The open log service (OPEN_LOG_SERVICE=ON) in concourse/scripts/build_tarball_open.bash only supports ROCKSDB as its log state and does not use the WITH_LOG_STATE parameter that was introduced to replace USE_ROCKSDB_LOG_STATE in other parts of the codebase.